Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Hysteria2 Protocol #2721

Open
wants to merge 98 commits into
base: master
Choose a base branch
from
Open

Add Hysteria2 Protocol #2721

wants to merge 98 commits into from

Conversation

JimmyHuang454
Copy link

Use hysteria's quic-go to support BBR and brutal congestion algorithm

Same as hysteria, BBR and Brutal congestion algorithm can improve performance.

Usage:

  1. send_mbps is needed when congestion's type is brutal, meaning the max bandwidth of current network can use to send.
{
  "transport":"quic",
  "transportSettings":{
      "congestion": {"type":"brutal", "send_mbps": 200}
  },
  "security":"tls",
  "securitySettings":{}
}
  1. If user don't know how to config brutal, it's just ok to use bbr in both client and server side.
{
  "transport":"quic",
  "transportSettings":{
      "congestion": {"type":"bbr"}
  },
  "security":"tls",
  "securitySettings":{}
}

Fix vprotogen bug and update all Protobuf data

vprotogen can not work, because it failed to recognize new version of protoc, so that we can not add new feature to v2ray.

Update go version to 1.21

fix #2701 #2644

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2023

Codecov Report

Attention: 1048 lines in your changes are missing coverage. Please review.

Comparison is base (cb84b28) 37.79% compared to head (0b7f037) 36.78%.
Report is 5 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2721      +/-   ##
==========================================
- Coverage   37.79%   36.78%   -1.01%     
==========================================
  Files         654      664      +10     
  Lines       38723    39771    +1048     
==========================================
- Hits        14636    14631       -5     
- Misses      22477    23524    +1047     
- Partials     1610     1616       +6     
Files Coverage Δ
app/dns/nameserver_quic.go 70.94% <ø> (ø)
common/protocol/quic/sniff.go 44.87% <ø> (ø)
config.pb.go 29.34% <ø> (ø)
transport/internet/quic/conn.go 58.00% <ø> (ø)
transport/internet/quic/hub.go 75.34% <100.00%> (+2.61%) ⬆️
...ransport/internet/quic/congestion/bbr/bandwidth.go 0.00% <0.00%> (ø)
transport/internet/quic/congestion/bbr/clock.go 0.00% <0.00%> (ø)
transport/internet/grpc/dial.go 3.27% <0.00%> (-0.06%) ⬇️
transport/internet/quic/congestion/utils.go 0.00% <0.00%> (ø)
transport/internet/quic/dialer.go 55.72% <63.33%> (-2.40%) ⬇️
... and 8 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JimmyHuang454
Copy link
Author

I had merged newest commit and fixed go-lint.

@xiaokangwang xiaokangwang added the Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval label Oct 28, 2023
@xiaokangwang
Copy link
Contributor

Hi! Thanks for your contribution. Since this merge request includes switching the dependency, it will be a longer review process, however I will try my best to expatiate the process.

@xiaokangwang
Copy link
Contributor

Hi, after some interal discuss, it seems there is no active maintainer within V2Ray could take over the maintaince of hysteria transport. Is it possible for you to refactor this code, so that the original quic transport is left as is, and make the necessary change to make hysteria a new transport that can be disabled when something goes wrong by removing the import line in https://github.com/v2fly/v2ray-core/blob/master/main/distro/all/all.go .

@xiaokangwang
Copy link
Contributor

I am happy to merge it after the usage of forked quic from hysteria is isolated from main quic.

@JimmyHuang454
Copy link
Author

I am happy to merge it after the usage of forked quic from hysteria is isolated from main quic.

Thanks. I will dig into it later. But I have been very busy lately, and I may reply very late. Anyway, I will try my best.

@JimmyHuang454
Copy link
Author

Android clients may use internet.UseAlternativeSystemDialer to replace v2ray system dialer (internet.DialSystem) to protect socket and prevent VPN traffic loopback. Currently hy2 does not comply with this. (Nevermind I found QUIC transport doesn't comply with this too, see #1510. V2Ray just lacks a similar API for PacketConn. Maybe ignore this for now...) QUIC transport uses internet.ListenSystemPacket instead. It seems that internet.ListenSystemPacket can be protected by internet.RegisterListenerController.

I mean something like the below.

Thanks, you are right. I forgot to handle that on the client side.

@CodingMoeButa
Copy link

建议保留brutal作为quic的一种流控算法选择,不必考虑它与hysteria的兼容性,反正hysteria已经被作为新的协议和传输层实现了。多一种选择不是坏事。

return int(quicvarint.Len(uint64(s)))
}

func (c *ConnWriter) writeTcpHeader() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be replaced by hyProtocol.WriteTCPRequest?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid it won't work because WriteTCPRequest() in hyProtocol executes i := VarintPut(buf, FrameTypeTCPRequest). However, this PR aims to separate the transport layer and the proxy layer, with FrameTypeTCPRequest being sent by the transport layer.

return newError("failed to send response").Base(err)
}

address := strings.Split(reqAddr, ":")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work for IPv6. Maybe use net.SplitHostPort instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. My bad.

func InitTLSConifg(streamSettings *internet.MemoryStreamConfig) (*hyClient.TLSConfig, error) {
tlsSetting := CheckTLSConfig(streamSettings, true)
if tlsSetting == nil {
tlsSetting = &tls.Config{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are ConfigFromStreamSettings and GetTLSConfig in github.com/v2fly/v2ray-core/v5/transport/internet/tls.

@dyhkwong
Copy link
Contributor

A lock is needed for RunningClient map[net.Destination](hyClient.Client) (like those in quic, grpc and http). I encountered some concurrent map writes panics.

By the way, identifying clients only by net.Destination is not enough. This will cause multiple outbounds with the same destination but different streamSettings using a wrong dialer. This is a common issue for quic, grpc and http, not only this PR's hysteria though.

@JimmyHuang454
Copy link
Author

A lock is needed for RunningClient map[net.Destination](hyClient.Client) (like those in quic, grpc and http). I encountered some concurrent map writes panics.

Ofcourse.

By the way, identifying clients only by net.Destination is not enough. This will cause multiple outbounds with the same destination but different streamSettings using a wrong dialer. This is a common issue for quic, grpc and http, not only this PR's hysteria though.

I have switched to using IP addresses to differentiate between different outbound connections. At the moment, it is indeed difficult to reuse a single port for multiple inbound connections.

@dyhkwong
Copy link
Contributor

Hi, I found that the values of congestion type, up_mbps and down_mbps are never used, so in fact it is always BBR no Brutal?

@JimmyHuang454
Copy link
Author

Hi, I found that the values of congestion type, up_mbps and down_mbps are never used, so in fact it is always BBR no Brutal?

My bad. Use the Brutal congestion algorithm when both up_mbps and down_mbps are not zero.

@dyhkwong
Copy link
Contributor

dyhkwong commented Jul 24, 2024

Is that Mbps or MBps? The multiplier should be 1000*1000/8 not 1000*1000. Hysteria's case-insensitive conversation is confusing...

https://github.com/apernet/hysteria/blob/b563f3981fc63eadb6710f50eecb92922b8ae1ce/app/cmd/client.go#L317

https://github.com/apernet/hysteria/blob/master/app/internal/utils/bpsconv.go#L10-L53

@JimmyHuang454
Copy link
Author

Is that Mbps or MBps? The multiplier should be 10001000/8 not 10001000. Hysteria's case-insensitive conversation is confusing...

https://github.com/apernet/hysteria/blob/b563f3981fc63eadb6710f50eecb92922b8ae1ce/app/cmd/client.go#L317

https://github.com/apernet/hysteria/blob/master/app/internal/utils/bpsconv.go#L10-L53

My bad again. It should be 1000*1000/8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quic-go@v0.33.0 can not be built on go 1.21
5 participants